@spencerkclark Thanks! I'll try to look into this tomorrow. The major interaction issues are with CFMaskCoder/CFScaleOffsetCoder and CFDatetimeCoder. So I do not expect too much issues with other coders here, but I'll check anyway.

Thanks @spencerkclark for taking a look at this! I left a couple of suggestions.

Incidentally, I think everyone will be happier the sooner we can relax the nanosecond precision restriction in Xarray :).

@kmuehlbauer let me know if you have any initial thoughts, particularly with respect to possible interaction with other coders.

@spencerkclark I do not immediately see any issues with other coders with the current implementation. Taking @shoyer's suggestion into account, we would need to make sure that xarray always gives preference to dtype-attribute instead of units (if both are given).

Looking at the implementation, maybe we don't need the separate new coder class and could keep this all in one objects?

I think the behavior could probably fit into a single coder:

• Encoding time units (to disk):
  • Convert data timedelta64 -> integer
  • Write both units and dtype attributes
• Decoding time units (from disk):
  • Convert data: integer -> timedelta64
  • If dtype != 'timedelta64', issue a future warning.

Thanks @shoyer—indeed it's probably simpler to have this all live in CFTimedeltaCoder, and we can re-use some existing code there. I went ahead and made that change.

The remaining awkward bit relates to interaction with masking and scaling. Since we are overloading the dtype encoding, we do not have a way of retaining the numerical dtype of the data that was written to disk during a round trip. I pushed a failing test in 503db4a to provide an example. Maybe I am just not thinking about this the proper way, however.

The remaining awkward bit relates to interaction with masking and scaling. Since we are overloading the dtype encoding, we do not have a way of retaining the numerical dtype of the data that was written to disk during a round trip. I pushed a failing test in 503db4a to provide an example. Maybe I am just not thinking about this the proper way, however.

For now I ended up punting on this in favor of forbidding providing any other encoding parameters when encoding timedeltas this new way. The previous encoding path supports this, and that functionality is still maintained in this PR.

Another approach would be to use a different name for the attribute we assign the timedelta dtype to, say "xarray_dtype" instead of "dtype", but this strays from how things are handled for boolean values, and I am not sure we are losing much by not supporting these other encoding parameters ("_FillValue", "missing_value", "add_offset", and "scale_factor") in this code path. I'm open to discuss, however.

So possibly this is closer now—I added some more tests and laid the groundwork for eventually turning off automatic decoding of variables with time-like units attributes (we would set CFTimedeltaCoder.decode_via_units to False by default instead of True). Ultimately though this is a delicate PR that should be carefully discussed / reviewed.

Just out of curiosity: How will a np.timedelta64("NaT") or np.timedelta64(None), whose .item() is None, be encoded? Because there's no int("nan") or something 🤔.

Also, will it ignore the units of NaT such as np.timedelta64("nat", "as")? I'm not sure what the use-case is, but for some reason NaT's can have a unit a in NumPy.

Thanks for taking an interest in this PR @jorenham—under the hood np.timedelta64("NaT") is represented by np.iinfo(np.int64).min, so it is straightforward to roundtrip with this type of encoding:

>>> arr = np.array([1, "NaT"], dtype="timedelta64[s]")
>>> arr.astype(np.int64)
array([                   1, -9223372036854775808])
>>> arr.astype(np.int64).astype("timedelta64[s]")
array([    1, 'NaT'], dtype='timedelta64[s]')

Though maybe for CF convention purposes we should record np.iinfo(np.int64).min as a "_FillValue" by default, and perhaps still allow specifying other fill values. While not necessary for xarray, it could be useful for downstream interpretation. I went ahead and implemented this in 46169ab. "_FillValue" or "missing_value" are now allowed in this encoding pathway—it is expected these will have the same dtype as the encoded values (int64)—while "add_offset" and "scale_factor" still are not.

The units will not be ignored, since the goal of this PR is to faithfully roundtrip np.timedelta64 values including their precise dtype.